-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release: fix koparse behavior on finding images 🖌 #3407
Conversation
/kind misc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you!
NIT: we might want to document which version of ko
this works with?
/lgtm
tekton/koparse/test_release.yaml
Outdated
"-logtostderr" | ||
"-stderrthreshold" | ||
"INFO" | ||
"-kubeconfig-writer-image" "gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/kubeconfigwriter@sha256:68453f5bb4b76c0eab98964754114d4f79d3a50413872520d8919a6786ea2b35" "-creds-image" "gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/creds-init@sha256:67448da79e4731ab534b91df08da547bc434ab08e41d905858f2244e70290f48" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes to validate, in tests, that this will work on release.yaml
generated by recent ko
while match is not None: | ||
image = match.group(0) | ||
images.append(image) | ||
line = re.sub(image, "found", line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it what is expected? to re.sub to found
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chmouel I… need to replace the found
match so that it doesn't loop forever when re matching things… It's not super sexy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah then perhaps using re.findall would be cleaner, something like this :
import re
YAML = """
- args: [
"-logtostderr",
"-stderrthreshold",
"INFO",
"-kubeconfig-writer-image", "gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/kubeconfigwriter@sha256:68453f5bb4b76c0eab98964754114d4f79d3a50413872520d8919a6786ea2b35", "-creds-ige", "gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/creds-init@sha256:67448da79e4731ab534b91df08da547bc434ab08e41d905858f2244e70290f48",
"-git-image",
"gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/git-init@sha256:7d5520efa2d55e1346c424797988c541327ee52ef810a840b5c6f278a9de934a",
]
"""
DIGEST_MARKER = "@sha256"
base = "gcr.io/tekton-releases/github.com/tektoncd/pipeline/cmd/"
pattern = re.compile(base + r"[0-9a-z\-]+" + DIGEST_MARKER + r":[0-9a-f]*")
for line in YAML.split("\n"):
findall = re.findall(pattern, line)
if findall:
print(findall)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but that can be done in a followup)
So this should work with all version 🙃 — the one that keep the format, and the one that do not 😝 |
Recent `ko` version *may* rework a bit the yaml and we *may* end up in having multiple image to find on the same line. This would make the release fail in those case. This fixes that by looking multiple times for the regular expression on a line. It also changes a bit the regular expression to target images better. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
f6a07cb
to
af2b556
Compare
/lgtm Looking Good! 🤙🏽 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
Recent
ko
version may rework a bit the yaml and we may end upin having multiple image to find on the same line. This would make the
release fail in those case.
This fixes that by looking multiple times for the regular expression
on a line.
It also changes a bit the regular expression to target images better.
Signed-off-by: Vincent Demeester vdemeest@redhat.com
/cc @afrittoli @bobcatfish @imjasonh @sbwsg @chmouel
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmd
dir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes